-
Notifications
You must be signed in to change notification settings - Fork 9
Utilize FourCIPP in MeshPy #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Utilize FourCIPP in MeshPy #354
Conversation
- The 4C input file now directly inherits from the FourCIPP input file - The dict and list comparison now also utilizes utilities from FourCIPP
(Only the second commit is crucial - the first commit is from #348) |
fabe8b3
to
e7afc73
Compare
Sorry for the chaos - got it working locally - I am missing something somewhere |
e7afc73
to
1b8dcc7
Compare
This was one really dumb typo on the fourcipp side ... Now everything works as expected. No more type casting needed for any numpy type and our functions can also be automatically converted everywhere. |
e9d49b9
to
de33f08
Compare
de33f08
to
b4cb809
Compare
@isteinbrecher now this would also be ready from a FourCIPP converter perspective. Now nothing should change anymore - therefore, now it's completely ready for review:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes @davidrudlstorfer ! This was actually very easy to review considering the complexity.
I really like how using the FourCIPP data structures simplifies a lot of the implementation in MeshPy BeamMe.
Some of my raised points can also be tackles in follow up PRs. We can shortly discuss this in the threads and open issues if we think this is the better way to go.
I am getting very used to the new interface!
@isteinbrecher this would be ready for your review again. I left four comments open for discussion and fixed the one thing. The remaining points should probably be tackled in separate PR's as they are already documented in the corresponding issue (testing tolerances, ...) |
With 4C-multiphysics/fourcipp#50 being merged I've switched back FourCIPP to the main repo again:) Therefore, this would be finally ready:) |
I've copied the ToDo's from #348 (comment)
After these things are fixed I would merge this and I noted the following clean-ups after this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot again @davidrudlstorfer for the changes, and sorry for my overdue review. I aded one more point to your comment #354 (comment).
I think if we only have two minor points left, I am confident that we can merge this soon!
Oh, and when this is finished, should we simply copy the open points from #354 (comment) into a "cleanup issue"? |
@isteinbrecher I've now reworked the testing stuff in a preliminary state. This state can be easily improved in a follow up. Currently only 4 tests (the abaqus tests) use the complicated string comparison. If we also switch that in the near future we can simplify everything a lot further. This would be ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, looks very good to me. There is only the one question regarding the inp
in testing remaining, then we can merge this 🚀.
Regarding the compare_string_with tolerances
, I had the exactly same thoughts, that we can remove it, but then ended up with the failing abaqus tests 😅.
fbd2218
to
ae689a6
Compare
Again ready for review - thanks for the review:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for going the extra mile @davidrudlstorfer .
Once this is merged, I will update the CubitPy PR.
Utilize FourCIPP in MeshPy (the input file inherits from it and also the testing uses a ready-to-use function from FourCIPP)